Skip to content

Make Helix Job Monitor uploads restart-safe - #17179

Open
mmitche wants to merge 6 commits into
dotnet:mainfrom
mmitche:copilot/job-monitor-upload-lifecycle
Open

Make Helix Job Monitor uploads restart-safe#17179
mmitche wants to merge 6 commits into
dotnet:mainfrom
mmitche:copilot/job-monitor-upload-lifecycle

Conversation

@mmitche

@mmitche mmitche commented Jul 23, 2026

Copy link
Copy Markdown
Member

Summary

  • separate queued/in-progress/failed uploads from durably completed, tagged runs
  • retry transient test-result downloads while avoiding replay of ambiguous AzDO writes
  • stop permanent or exhausted uploads without hanging or changing Helix pass/fail
  • make transient per-file download failures retryable without skipping remaining files
  • add runner, adapter, publisher, and Helix service coverage

Stacking

This PR is stacked on #17178. Until that PR merges, its two documentation commits appear in this PR as well; the implementation commit is Make Job Monitor uploads restart-safe.

Closes #17173
Parent epic: #17171

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Helix Job Monitor’s Azure DevOps test-result upload pipeline so it can be safely restarted after crashes/timeouts by separating durable completion (tagged runs) from queued/in-progress/failed uploads, and by bounding/targeting retries to avoid replaying ambiguous Azure DevOps writes.

Changes:

  • Introduces explicit upload lifecycle state tracking (queued/in-progress/durably-completed/failed) and updates the runner/queue to honor durable completion tags.
  • Adds transient-failure classification and bounded retries for safe (read-only) download operations while disabling retries for state-changing Azure DevOps operations.
  • Expands unit coverage across runner, services, publisher, and fakes for restart-safety and transient/permanent failure behaviors.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/JobMonitorRunnerTests.cs Adds/updates scenarios asserting restart-safe behavior and non-replay of ambiguous uploads/completions.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/HelixServiceTests.cs Adds coverage for transient per-file download failures while still attempting the full batch.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/Fakes/FakeHelixService.cs Adds queued transient download failure injection to simulate retries.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/Fakes/FakeAzureDevOpsService.cs Adds create/complete failure injection and tracks complete call counts for new behaviors.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/AzureDevOpsServiceTests.cs Verifies create/complete do not retry ambiguous writes.
src/Microsoft.DotNet.Helix/Sdk.Tests/Microsoft.DotNet.Helix.Sdk.Tests/AzureDevOpsResultPublisherTests.cs Adds tests for configurable retry behavior for ambiguous writes.
src/Microsoft.DotNet.Helix/JobMonitor/TransientFailureDetector.cs New shared transient-failure classifier used by queue/services.
src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadQueue.cs Splits upload into phases with bounded retries for safe operations and non-replay of ambiguous writes.
src/Microsoft.DotNet.Helix/JobMonitor/Services/HelixService.cs Treats transient per-file failures as retryable after attempting remaining files; throws aggregated transient failure.
src/Microsoft.DotNet.Helix/JobMonitor/Services/AzureDevOpsService.cs Disables adapter-level transient retries for create/complete/attachment writes; disables publisher write retries; improves HttpRequestException status propagation.
src/Microsoft.DotNet.Helix/JobMonitor/MonitorState.cs Replaces “processed” set with explicit upload lifecycle state machine keyed by Helix job.
src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorRunner.Design.md Updates behavioral spec to document restart-safe semantics and retry/non-replay rules.
src/Microsoft.DotNet.Helix/JobMonitor/JobMonitorRunner.cs Avoids marking jobs as processed at queue time; prevents duplicate enqueue within an invocation via state.
src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/Model/AzureDevOpsReportingParameters.cs Adds RetryWrites parameter to control write retry behavior.
src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/AzureDevOpsResultPublisher.cs Implements configurable retry count based on request method / write-retry configuration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/AzureDevOpsResultPublisher.cs Outdated
@mmitche
mmitche marked this pull request as ready for review July 23, 2026 21:12
premun
premun previously approved these changes Jul 27, 2026
Copilot AI review requested due to automatic review settings July 27, 2026 16:36

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/AzureDevOpsResultPublisher.cs:553

  • GetRetryCount checks method == HttpMethod.Get, which can miss logically-GET methods created via new HttpMethod("GET") (or other equivalent instances). This can incorrectly disable retries for GET requests when RetryWrites is false; compare by value instead of reference.
    internal static int GetRetryCount(HttpMethod method, bool retryWrites)
        => retryWrites || method == HttpMethod.Get ? 10 : 0;

Comment thread src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadQueue.cs Outdated
Copilot AI review requested due to automatic review settings July 27, 2026 16:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/Microsoft.DotNet.Helix/AzureDevOpsTestPublisher/AzureDevOpsResultPublisher.cs:553

  • GetRetryCount uses method == HttpMethod.Get, which is reference equality for HttpMethod. This returns false for new HttpMethod("GET") (and any non-cached instance), disabling retries for safe read-only GETs and causing the new unit test to fail. Use Equals (or compare method.Method) instead.
    internal static int GetRetryCount(HttpMethod method, bool retryWrites)
        => retryWrites || method == HttpMethod.Get ? 10 : 0;

Track upload lifecycle explicitly, retry only safe read operations, avoid replaying ambiguous writes, and keep incomplete runs untagged for later recovery.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: baa969a4-1d79-4b6a-a3f0-4d349d768b5e
Copilot AI review requested due to automatic review settings July 28, 2026 14:56
@mmitche
mmitche force-pushed the copilot/job-monitor-upload-lifecycle branch from 11e4992 to 6c5f6ef Compare July 28, 2026 14:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadQueue.cs:27

  • The file header summary still says uploads have “indefinite retry on transient errors”, but the new implementation retries only bounded read-only operations and attempts state-changing AzDO operations once (to avoid replaying ambiguous writes). Updating the summary here will prevent confusion later.
    /// </summary>
    internal sealed class TestResultUploadQueue
    {
        private const int MaximumTransientAttempts = 3;
        private const string AzdoWarningPrefix = "##vso[task.logissue type=warning]";

src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadQueue.cs:225

  • The warning log message says “The run remains untagged … may retry the upload” even when the failure happens before any test run is created (testRunId=0), which is confusing. Consider emitting a different message when no run exists yet.
                    _logger.LogWarning(ex,
                        "{Prefix}Failed to {OperationDescription} for job {JobName}. Test run ID was {TestRunId}. "
                        + "{FailureKind} The run remains untagged and a later monitor invocation may retry the upload.",
                        AzdoWarningPrefix,
                        operationDescription,

premun
premun previously approved these changes Jul 28, 2026
Pass retry counts explicitly, use the shared retry helper with transient classification, and preserve reporting parameter compatibility while disabling ambiguous write retries.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: baa969a4-1d79-4b6a-a3f0-4d349d768b5e
Copilot AI review requested due to automatic review settings July 28, 2026 19:35
Describe bounded read retries and single-attempt writes accurately.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: baa969a4-1d79-4b6a-a3f0-4d349d768b5e
@mmitche

mmitche commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

Addressed the retry feedback in c7684719e and 4daa22dfb:

  • A monitor timeout/cancellation is not retried: the shared helper checks the monitor token and rethrows OperationCanceledException before applying the transient predicate. A TaskCanceledException is only retryable when the monitor token itself was not cancelled, such as an independent HTTP timeout.
  • RetryWrites is now in the record constructor list. The original five-argument constructor and five-value Deconstruct remain for source/binary compatibility, and JSON constructor selection is explicit.
  • Job Monitor writes are deliberately not retried because a request can reach Azure DevOps and mutate state before the client observes a timeout/disconnect. Replaying can create duplicate runs, results, or attachments. The run remains untagged so a later monitor invocation can recover it through the durable workflow.
  • SendWithRetryAsync now receives an explicit retry count.
  • Upload execution now has a zero-retry TryExecuteAsync path and passes an explicit retry count to TryExecuteWithRetryAsync.
  • The upload queue now uses the shared RetryHelper, extended with a retry predicate, explicit retry count, exponential delay, cancellation handling, and retry callback.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadQueue.cs:235

  • The warning message assumes "The run remains untagged" even when no test run has been created yet (e.g., download failures with testRunId==0), and it also asserts an untagged state that may not be true after an ambiguous completion/tagging failure. This can mislead troubleshooting; consider wording that’s conditional/hedged while still indicating that a later monitor invocation may retry the upload.
                        ? "Transient retry limit reached."
                        : "The operation may have partially completed and is not safe to replay in this invocation."
                    : "The failure is not retryable.";
                _logger.LogWarning(ex,
                    "{Prefix}Failed to {OperationDescription} for job {JobName}. Test run ID was {TestRunId}. "

Copilot AI review requested due to automatic review settings July 28, 2026 19:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadQueue.cs:235

  • The warning message here always mentions a "test run" and "The run remains untagged" even when testRunId is 0 (e.g., during the download phase, before any run is created). That makes the log misleading and can confuse diagnosing failures.

Consider tailoring the message when no run exists yet (or omitting the test-run clause entirely for non-AzDO operations).

                _logger.LogWarning(ex,
                    "{Prefix}Failed to {OperationDescription} for job {JobName}. Test run ID was {TestRunId}. "
                    + "{FailureKind} The run remains untagged and a later monitor invocation may retry the upload.",
                    AzdoWarningPrefix,
                    operationDescription,

src/Microsoft.DotNet.Helix/JobMonitor/MonitorState.cs:170

  • AddProcessedHelixJobs seeds durable completion state from existing AzDO tags, but it does not increment _processedJobCount. As a result, StatusReporter will treat these jobs as processed (via IsHelixJobProcessed), while the final summary uses ProcessedJobCount and may report 0 processed jobs even when everything was already processed before this invocation.
        public void AddProcessedHelixJobs(IEnumerable<string> jobNames)
        {
            lock (_sync)
            {
                foreach (string jobName in jobNames)
                {
                    _testResultUploadStates[jobName] = TestResultUploadState.DurablyCompleted;
                }

Make RetryWrites a normal optional record constructor parameter without compatibility scaffolding for this newly introduced option.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: baa969a4-1d79-4b6a-a3f0-4d349d768b5e
Copilot AI review requested due to automatic review settings July 28, 2026 20:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/Microsoft.DotNet.Helix/JobMonitor/TestResultUploadQueue.cs:235

  • The warning emitted on failure always says "The run remains untagged" even when the failure occurs before any test run exists (e.g., download failures where testRunId is 0). This can mislead diagnosis in AzDO logs. Consider wording this in terms of "durable tagged completion" rather than implying an existing run.
                _logger.LogWarning(ex,
                    "{Prefix}Failed to {OperationDescription} for job {JobName}. Test run ID was {TestRunId}. "
                    + "{FailureKind} The run remains untagged and a later monitor invocation may retry the upload.",
                    AzdoWarningPrefix,

mmitche added 2 commits July 28, 2026 14:06
Apply explicit retry counts and transient classification through Microsoft.Arcade.Common.ExponentialRetry instead of extending the publisher retry helper.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: baa969a4-1d79-4b6a-a3f0-4d349d768b5e
Delete the unused metadata pipeline and its private helpers rather than maintaining commented, nonfunctional scaffolding.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: baa969a4-1d79-4b6a-a3f0-4d349d768b5e
Copilot AI review requested due to automatic review settings July 28, 2026 21:06
@mmitche

mmitche commented Jul 28, 2026

Copy link
Copy Markdown
Member Author

Updated in \70f6a1c37\ and \31dff43a7: upload read retries now use \Microsoft.Arcade.Common.ExponentialRetry, with the callback deciding whether a transient failure is retryable and explicit retry-count semantics retained. The custom \RetryHelper\ extension was removed. I also deleted the inactive test-metadata method, commented block, and private helpers rather than maintaining nonfunctional scaffolding.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Job Monitor: make test-result upload lifecycle explicit and resilient

3 participants